-
Notifications
You must be signed in to change notification settings - Fork 704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
remove some of the reliance on the exp pkg #2405
remove some of the reliance on the exp pkg #2405
Conversation
utils/sorting.go
Outdated
if a.Less(b) { | ||
return -1 | ||
} | ||
return 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this correctly handles the case that a
is equal to b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ill add that case :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
took another look at the code... seems there are a few options
- add an addition method to the Sortable interface
Equals(T) bool
and implement that on all types that have theLess
Method implemented - take the slight performance hit of returning a -1, or 1 ... since returning 0 would just skip a swap operation. which is how it currently operates
- We change the
Less
method to be similar toCompare
and update that on all implementations
let me know if i might be missing some easier approach and if you have a preference on which i should move forward on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use sort.Slice
instead?
func Slice(x any, less func(i, j int) bool)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sort.Slice
uses reflection and is slower than slices.SortFunc
/ slices.Sort
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I like option 3 the best. Something like this seems reasonable:
type Sortable[T any] interface {
Compare(T) int
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like we will have to keep using the exp
pkg here... i thought that the slices update was in 1.20. looks like its in 1.21. I will update that and just move forward with option 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had originally thought we would just do:
if a.Less(b) {
return -1
}
if b.Less(a) {
return 1
}
return 0
But changing the Sortable
interface is a better long term strategy imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the go version change - we likely won't update to 1.21
until 1.22
is released
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is trivial to implement sort.Interface
:
type sortables[T Sortable[T]] []T
func (s *sortables[T]) Len() int { return len(*s) }
func (s *sortables[T]) Less(i, j int) bool { return (*s)[i].Less((*s)[j]) }
func (s *sortables[T]) Swap(i, j int) { (*s)[i], (*s)[j] = (*s)[j], (*s)[i] }
Then you can just cast and call: sort.Sort(sortables(s))
this seems like it might be a bigger task
|
I can take a stab at updating the |
That is fine with me. Just for context this was preventing some upgrades |
Closing this in favor of: #2424. Thanks for suggesting this! |
Notes:
x/exp
pkgWhy this should be merged
func(i, j T) bool
tofunc(i, j T) int
so upgrading now will prevent issues in the futureHow this works
How this was tested